Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(metric): Metric visualization #1658

Merged
merged 35 commits into from
Jun 10, 2022
Merged

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Apr 22, 2022

Summary

This PR integrate a new chart type: Metric
The metric can be configured to be rendered with a static fully colored background or a small vertical/horizontal bar showing some kind of progress of the main metric value across a defined domain.
As alternative to the general progress, you can supply a trend dataset to be rendered on the background of a fully colored metric panel.

The panels can be arranged into a row, columns or grid, depending on how the data prop array of arrays is configured.
Empty space can be defined but living an undefined element in the data array.

Non finite values are represented by a N/A string that can be configured with the theme.
The color used for text depends on the current progress configuration and the background color, alternating between a light and dark font color depending on the respective background.

Screenshot 2022-05-19 at 13 45 16

Screenshot 2022-05-19 at 13 45 33

Implemented, but not exposed yet, is the ability to have a full width progress bar. It is not yet exposed because we first need to solve how to deal with text living half within a colored background and half within the panel background. Some tests are done with the CSS property mix-blend-mode or by overlapping the same text with a different color and clipping one of them ad the same height of the background progress bar.

The main props for this metric are:

  data: (MetricOnly | MetricWProgress | MetricWTrend | undefined)[][];

data is a 2d array where each element represent a cell. The outer array represent the rows, and each inner array represent columns.
This shows a single row: [[d1,d2,d3]]
For a single column instead: [[d1],[d2],[d3]]
For a grid: [[d1,d2,d3],[d4,d5,d6],[d7,d8,d9]]

Each cell can be configured separately as one of the following MetricBase | MetricWProgress | MetricWTrend described as:

// Always present on every cell
export type MetricBase = {
  value: number; // for not available/null/undefined you should convert it to NaN
  color: Color;
  valueFormatter: (d: number) => string;
  title?: string;
  subtitle?: string;
  extra?: ReactElement; // this is the small text above the metric value, should always be a limited length and style HTML element
};

/** @alpha */
export type MetricWProgress = MetricBase & {
  domain: [min: number, max: number]; // this is required to show a progress bar
  progressBarDirection?: LayoutDirection;
};

/** @alpha */
export type MetricWTrend = MetricBase & {
  trend: { x: number; y: number }[]; // a simple continuous trend
  trendShape?: 'area' | 'bar'; // the shape of the trend, area by default
  trendA11yTitle?: string; // a title used for screen readers to read out loud the trend
  trendA11yDescription?: string; // a description text for screen readers that describes the trend 
};

The props progressBarMode and progressBarOrientation are considered globally across the chart. The reasons are:
- text needs to be aligned across and adding the ability to change per cell the progress bar mode and orientation pushes us to find the minimum common alignment of every configuration (not too difficult but inconvenient)
- having a mix of configurations, vertical, horizontal, full or small can cause confusions and makes the comparison, where possible, even harder. Some horizontal bars can be confused with a vertical short full bar. (I know the full bar is not exposed yet, but this can make it harder to switch back once released).
The previous description is no more valid. Together with @gvnmagni we decided that we don't want to limit the possibility of having horizontal and vertical progress bar together, and this should be used carefully buy the consumer.
Also having a progress bar together with a trend doesn't force the alignment anymore, because it seems that benefits have the same weight of disadvantages.

cc @elastic/eui-design a new set of style was added to the theme. Could you double-check the values together with @gvnmagni ? Happy to use fewer hardcoded color values and reuse EUI variables.

Details

Possible future enhancements:

  • Add an array of color ranges to pick the right color
  • Re-enable the full bar (as alternative to the small bar) when the color contrast with text is resolved

Issues

close #1578

Checklist

  • The :theme label has been added and the @elastic/eui-design team has been pinged when there are Theme API changes
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

@cla-checker-service
Copy link

cla-checker-service bot commented May 18, 2022

💚 CLA has been signed

@markov00 markov00 added :new chart type New chart type related feature request :theme labels May 19, 2022
@markov00 markov00 marked this pull request as ready for review May 19, 2022 14:41
@markov00 markov00 requested review from monfera and nickofthyme May 20, 2022 07:28
@cchaos
Copy link
Contributor

cchaos commented May 23, 2022

@markov00 Can you be more specific or are you just looking for EUI token based equivalents to

metric: {
text: {
lightColor: '#E0E5EE',
darkColor: '#343741',
},
barBg: '#EDF0F5',
background: '#FFFFFF',
nonFiniteText: 'N/A',
},

I don't see EUI tokens used elsewhere in that file, so my guess is hex is fine as the default? Or are you looking specifically for better color guidance from @gvnmagni?

@markov00
Copy link
Member Author

@markov00 Can you be more specific or are you just looking for EUI token based equivalents to

I don't see EUI tokens used elsewhere in that file, so my guess is hex is fine as the default? Or are you looking specifically for better color guidance from @gvnmagni?

I think this is fine for now since the EUI themes anyway overwrite this theme.
I know that @gvnmagni is going to find the right EUI token to replace those in EUI chart themes

@cchaos
Copy link
Contributor

cchaos commented May 24, 2022

Great! @gvnmagni Let me know if you need help getting the EUI theme updated.

@gvnmagni
Copy link

Thanks @cchaos! It should be quite straightforward but I'll ping you in case of need 🙂

@shahzad31
Copy link
Contributor

Can we please add a tooltip option for description, often time a metric value can be hard to explain in title, so an added description is needed, for example
for example we would like to replace these euistate with new metric vis from lens, but we need description tooltip to explain what these metrics are

image

@gvnmagni
Copy link

Thank you Shahzad, you raised a very good point! I'll see how to make this work, thanks again for your help!

@markov00
Copy link
Member Author

Thanks @shahzad31, having a tooltip to add additional information is definitely useful, but I'm also curious why we need to provide such information in your case: if I'm using the Observability solution measuring my web page performance I should know what FCP, LCP,total blocking time means, if I don't know very well them by first hand I can't interpret information showing in this app, I can't gain insight, and I can't take actions. If you need a glossary with general terms and acronyms you can probably direct the user to a doc page, but probably polluting the UI with these information is not the best solution.
IMHO I see how this tooltip could help in a getting started situation (like a simple web app to measure the performance of your website, where the user is an inexperience one and needs a bit of guidante), but in the long run, when using professionally our solution, this is no longer useful and makes only noise in the page.

@shahzad31
Copy link
Contributor

Thanks @shahzad31, having a tooltip to add additional information is definitely useful, but I'm also curious why we need to provide such information in your case: if I'm using the Observability solution measuring my web page performance I should know what FCP, LCP,total blocking time means, if I don't know very well them by first hand I can't interpret information showing in this app, I can't gain insight, and I can't take actions. If you need a glossary with general terms and acronyms you can probably direct the user to a doc page, but probably polluting the UI with these information is not the best solution.
IMHO I see how this tooltip could help in a getting started situation (like a simple web app to measure the performance of your website, where the user is an inexperience one and needs a bit of guidante), but in the long run, when using professionally our solution, this is no longer useful and makes only noise in the page.

That's a valid point, I agree that it's really useful in case of on boarding a user to a prebuilt dashboard, like we have in observability solutions. But maybe for that use case , have a setup guide or tour component is probably better than a tooltip description. Though I think where a complex formula is involved, it is helpful to show description tooltip. Inline Guides are always useful compared to directing users to docs page. Even if we have a docs page, we will need to add a link to docs page. That will pose another challenge. In any case, I wanted to highlight this. If you want to see it being used. You can visit user experience dashboard in observability. There we are using these tooltips on bunch of metrics

}
const [xMin, xMax] = extent(trend.map((d) => d.x));
const [, yMax] = extent(trend.map((d) => d.y));
const xScale = (value: number) => (value - xMin) / (xMax - xMin);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What sort of range are we talking about here?
I'm particularly interested in the impact on loading and painting times with and without sparklines.
In the early introduction of sparklines in our APM solution, they caused a significant degradation for some of our heavier observability use cases.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Tina, thanks for asking.
The sparkline is considered just an embellishment, not something to be used to understand precise trends and values, so the consumer needs to be wise on them and doesn't push for rendering "nanosecond precision pixels in a time window of days".

This sparkline construction is limited to the bare minimum: we using just a simple linear scale, removing the needs of time-zone aware time scale, the is no nice fallback for missing data points, no styling options (expect for the interpolating curve type), no interaction, no rendered data points, no value formatting etc, just a mere data point projection to a normalized space as as the following:

  • the min x and max x values are mapped into [0,1]
  • the y values are mapped into [0, 1] where o is always zero and 1 represents the y max

So basically, loading and painting time really depends on factors that we don't control and should not control:

number of data points to render:
you should not throw more than 30 max data points into such a chart. This translates into an SVG with a simple rect for the background and a path with 30 values. From the rendering point of view it can be compared to a simple SVG icon, as we already have on most interfaces.
These are for example the comparison between the SVG of the spark area vs the SVG of the SnowFlake icon in EUI
Screenshot 2022-06-03 at 10 26 09
Screenshot 2022-06-03 at 10 26 39

The painting time is negligible. A different argument is if you are sending the chart 1k data points to render. This is a bad practice and we discourage that usage. It doesn't provide any significant readability improvement over rendering 100 or fewer data points. The sparkline can be considered as an infographic where the precision of the values is not what we are looking for.

We should not control this factor, because it is not our task to re-aggregate, split, summarize, sample, your data. You have the control, you can make the right decision based on your environment (time window, data complexity, priorities, query environment, visual aspect, number of charts to render etc).

how you query your data:
you should add a sparkline only if it is really necessary or if the cost of query your data is relatively small or negligible: for example when you already have that data in a cache, or it's used somewhere else in your page or viz, or you still need to query your data in that way. The TSVB metric for example, under a specific configuration, already queries a full time series to render just the last bucket of it (I'm not saying that TSVB is correct, but just saying that we don't need to issue another query just for that).
If the calculation to display that metric is complex, or your time interval is too big or your data is too much, it's probably better to don't show the sparkline if you care about performances. But that really depends on you, the consumer, the App/Solution developer/designer to consider if such a query and such small infographic chart is so important visually to require a data fetch, or you can just live with a simple metric.

Note also the following: in the Observability solution, there are multiple places where multiple sparklines are rendered using elastic-charts with a minimal overhead on the processing and rendering side. Those charts are rendered in canvas instead of SVG, but IMHO their number is so low and the data points are so limited that there is no major benefit in one or the other approach
Screenshot 2022-06-03 at 10 39 08

We can also move the rendering to a Canvas renderer, but I don't see the need right now for such optimization. Happy to rediscuss my idea if, with the above consideration, rendering those SVG becomes a performance bottleneck.

Also here, we should not control that, we can't as we are just a library that is data source agnostic.
What we can control is the rendering method, and we are happy to reconsider it.

@1Copenut
Copy link

1Copenut commented Jun 2, 2022

I'm taking a look at this today for accessibility. I love this new display and how much information it packs in small grids. I'll provide feedback in a review comment late US business hours or tomorrow morning.

Copy link

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a look at the branch on my local machine today. I ran it through VoiceOver on Safari (MacOS Monterrey) to get a sense what information is being read out.

I refactored the HTML slightly to improve the screen reader experience, testing as I went. I'll add the modified HTML in comments so I can properly annotate changes without overloading the PR comment message.

Please consider these as collaborative suggestions. Happy to chat further or answer any questions.

@1Copenut
Copy link

1Copenut commented Jun 2, 2022

Proposed sparkline metric HTML

! I moved the sparkline after the text and used `z-index` to flip the stacking order
! This creates a situation where users can listen to the information, then hear the SVG extended description
! There is an open question about can we create basic templates and insert values to build up a description

<div
+ aria-labelledby="metric-sparkline-title"
  class="echMetric echMetric--rightBorder echMetric--bottomBorder echMetric--small echMetric--vertical"
+ role="figure"
  style="background-color: rgb(255, 255, 255);"
>
  <!-- Textual data -->
  <div class="echMetricText echMetricText--small echMetricText--vertical" style="color: rgb(52, 55, 65);">
    <div>
+ <h2 class="echMetricText__title echMetricText__titleS" id="metric-sparkline-title">
        Cloud Revenue
+ <span class="echMetricText__subtitle">Quarterly</span>
      </h2>
    </div>
    <div class="echMetricText--bottom-right">
        <p class="echMetricText__value echMetricText__valueS">
+         <span aria-hidden="true">
+           <span class="echMetricText__unit echMetricText__unit--large">$323.57</span>
+           <span class="echMetricText__unit">k</span>
+         </span>
+         <span class="sr-only">Q2 Fiscal year 2022: Three-hundred twenty-three point fifty-seven. Thousand US
              dollars.</span>
          </p>
          <p class="echMetricText__extra">
+           <span aria-hidden="true" class="echMetricText__unit echMetricText__unit--light">
+            <span>This Year 10M</span>
            </span>
+           <span class="sr-only">This year: Ten million US dollars.</span>
        </p>
      </div>
    </div>

    <!-- Sparkline -->
    <!-- https://www.smashingmagazine.com/2021/05/accessible-svg-patterns-comparison/-->
    <div class="echSingleMetricSparkline">
      <svg
        class="echSingleMetricSparkline__svg"
        height="100%" preserveaspectratio="none"
        viewbox="0 0 1 1"
        width="100%"
+       role="img"
+       aria-labelledby="svg-title svg-description"
      >
+         <title id="svg-title">Cloud revenue fiscal year 2022.</title>
+         <text id="svg-description" class="sr-only" font-size="0">
            Revenue has increased 7.23% year over year. The year low was three-hundred fifteen thousand US dollars on
            September 1, 2021. The year high was three-hundred forty-one thousand US dollars.
          </text>
          <rect fill="#F1D86F" height="1" width="1" x="0" y="0"></rect>
          <path d="M0,0.39200761179828736L0.008403361344537815,0.4081826831588963L0.01680672268907563,0.4348239771646052L0.025210084033613446,0.4471931493815414L0.03361344537815126,0.4524262607040913L0.04201680672268908,0.41294005708848713L0.05042016806722689,0.3805899143672693L0.058823529411764705,0.36536631779257844L0.06722689075630252,0.3529971455756422L0.07563025210084033,0.3672692673644149L0.08403361344537816,0.3648905803996194L0.09243697478991597,0.3829686013320647L0.10084033613445378,0.39200761179828736L0.1092436974789916,0.4019980970504282L0.11764705882352941,0.40247383444338725L0.12605042016806722,0.4043767840152236L0.13445378151260504,0.3529971455756422L0.14285714285714285,0.38534728829686016L0.15126050420168066,0.41817316841103713L0.15966386554621848,0.4281636536631779L0.16806722689075632,0.4509990485252141L0.17647058823529413,0.4590865842055185L0.18487394957983194,0.4300666032350142L0.19327731092436976,0.41960038058991433L0.20168067226890757,0.27259752616555666L0.21008403361344538,0.2797335870599429L0.2184873949579832,0.1194100856327307L0.226890756302521,0L0.23529411764705882,0.044719314938154175L0.24369747899159663,0.11322549952426264L0.25210084033613445,0.16270218839200756L0.2605042016806723,0.21836346336822077L0.2689075630252101,0.25642245480494763L0.2773109243697479,0.2916270218839201L0.2857142857142857,0.3292102759276879L0.29411764705882354,0.36774500475737393L0.3025210084033613,0.4067554709800191L0.31092436974789917,0.44909609895337776L0.31932773109243695,0.39581351094196005L0.3277310924369748,0.3977164605137964L0.33613445378151263,0.3701236917221694L0.3445378151260504,0.38249286393910564L0.35294117647058826,0.36964795432921027L0.36134453781512604,0.3739295908658421L0.3697478991596639,0.37868696479543296L0.37815126050420167,0.3729781160799239L0.3865546218487395,0.33777354900095147L0.3949579831932773,0.3011417697431018L0.40336134453781514,0.3006660323501428L0.4117647058823529,0.3273073263558516L0.42016806722689076,0.17602283539486208L0.42857142857142855,0.19695528068506185L0.4369747899159664,0.2507136060894386L0.44537815126050423,0.30589914367269266L0.453781512605042,0.3220742150333016L0.46218487394957986,0.3063748810656518L0.47058823529411764,0.1898192197906755L0.4789915966386555,0.1646051379638439L0.48739495798319327,0.18601332064700282L0.4957983193277311,0.18220742150333014L0.5042016806722689,0.18791627021883917L0.5126050420168067,0.2402473834443387L0.5210084033613446,0.263558515699334L0.5294117647058824,0.3011417697431018L0.5378151260504201,0.3101807802093245L0.5462184873949579,0.33634633682207427L0.5546218487394958,0.3477640342530923L0.5630252100840336,0.38772597526165553L0.5714285714285714,0.4134157944814463L0.5798319327731093,0.4310180780209324L0.5882352941176471,0.4400570884871551L0.5966386554621849,0.4134157944814463L0.6050420168067226,0.4086584205518554L0.6134453781512605,0.38392007611798284L0.6218487394957983,0.3805899143672693L0.6302521008403361,0.38772597526165553L0.6386554621848739,0.3658420551855376L0.6470588235294118,0.3591817316841104L0.6554621848739496,0.07802093244529018L0.6638655462184874,0.09229305423406275L0.6722689075630253,0.11750713606089436L0.680672268907563,0.1684110371075167L0.6890756302521008,0.20599429115128454L0.6974789915966386,0.263558515699334L0.7058823529411765,0.31113225499524266L0.7142857142857143,0.34681255946717415L0.7226890756302521,0.38106565176022833L0.7310924369747899,0.42483349191246433L0.7394957983193278,0.35775451950523307L0.7478991596638656,0.3220742150333016L0.7563025210084033,0.33206470028544244L0.7647058823529411,0.33777354900095147L0.773109243697479,0.3477640342530923L0.7815126050420168,0.3634633682207421L0.7899159663865546,0.35775451950523307L0.7983193277310925,0.3501427212178877L0.8067226890756303,0.2825880114176974L0.8151260504201681,0.2954329210275928L0.8235294117647058,0.32017126546146524L0.8319327731092437,0.31874405328258804L0.8403361344537815,0.3382492863939106L0.8487394957983193,0.3582302568981922L0.8571428571428571,0.38915318744053284L0.865546218487395,0.41151284490960993L0.8739495798319328,0.4467174119885823L0.8823529411764706,0.4576593720266413L0.8907563025210085,0.4243577545195052L0.8991596638655462,0.3477640342530923L0.907563025210084,0.346336822074215L0.9159663865546218,0.351569933396765L0.9243697478991597,0.3591817316841104L0.9327731092436975,0.38011417697431016L0.9411764705882353,0.41864890580399616L0.9495798319327731,0.4191246431969553L0.957983193277311,0.42768791627021885L0.9663865546218487,0.4386298763082779L0.9747899159663865,0.2283539486203615L0.9831932773109243,0.264034253092293L0.9915966386554622,0.30732635585157L1,0.3396764985727878L1,1L0.9915966386554622,1L0.9831932773109243,1L0.9747899159663865,1L0.9663865546218487,1L0.957983193277311,1L0.9495798319327731,1L0.9411764705882353,1L0.9327731092436975,1L0.9243697478991597,1L0.9159663865546218,1L0.907563025210084,1L0.8991596638655462,1L0.8907563025210085,1L0.8823529411764706,1L0.8739495798319328,1L0.865546218487395,1L0.8571428571428571,1L0.8487394957983193,1L0.8403361344537815,1L0.8319327731092437,1L0.8235294117647058,1L0.8151260504201681,1L0.8067226890756303,1L0.7983193277310925,1L0.7899159663865546,1L0.7815126050420168,1L0.773109243697479,1L0.7647058823529411,1L0.7563025210084033,1L0.7478991596638656,1L0.7394957983193278,1L0.7310924369747899,1L0.7226890756302521,1L0.7142857142857143,1L0.7058823529411765,1L0.6974789915966386,1L0.6890756302521008,1L0.680672268907563,1L0.6722689075630253,1L0.6638655462184874,1L0.6554621848739496,1L0.6470588235294118,1L0.6386554621848739,1L0.6302521008403361,1L0.6218487394957983,1L0.6134453781512605,1L0.6050420168067226,1L0.5966386554621849,1L0.5882352941176471,1L0.5798319327731093,1L0.5714285714285714,1L0.5630252100840336,1L0.5546218487394958,1L0.5462184873949579,1L0.5378151260504201,1L0.5294117647058824,1L0.5210084033613446,1L0.5126050420168067,1L0.5042016806722689,1L0.4957983193277311,1L0.48739495798319327,1L0.4789915966386555,1L0.47058823529411764,1L0.46218487394957986,1L0.453781512605042,1L0.44537815126050423,1L0.4369747899159664,1L0.42857142857142855,1L0.42016806722689076,1L0.4117647058823529,1L0.40336134453781514,1L0.3949579831932773,1L0.3865546218487395,1L0.37815126050420167,1L0.3697478991596639,1L0.36134453781512604,1L0.35294117647058826,1L0.3445378151260504,1L0.33613445378151263,1L0.3277310924369748,1L0.31932773109243695,1L0.31092436974789917,1L0.3025210084033613,1L0.29411764705882354,1L0.2857142857142857,1L0.2773109243697479,1L0.2689075630252101,1L0.2605042016806723,1L0.25210084033613445,1L0.24369747899159663,1L0.23529411764705882,1L0.226890756302521,1L0.2184873949579832,1L0.21008403361344538,1L0.20168067226890757,1L0.19327731092436976,1L0.18487394957983194,1L0.17647058823529413,1L0.16806722689075632,1L0.15966386554621848,1L0.15126050420168066,1L0.14285714285714285,1L0.13445378151260504,1L0.12605042016806722,1L0.11764705882352941,1L0.1092436974789916,1L0.10084033613445378,1L0.09243697478991597,1L0.08403361344537816,1L0.07563025210084033,1L0.06722689075630252,1L0.058823529411764705,1L0.05042016806722689,1L0.04201680672268908,1L0.03361344537815126,1L0.025210084033613446,1L0.01680672268907563,1L0.008403361344537815,1L0,1Z"
        fill="rgba(246, 229, 157, 1)"
        stroke="none"
        stroke-width="0"
        transform="translate(0, 0.5),scale(1,0.5)"
      ></path>
    </svg>
  </div>
</div>

@1Copenut
Copy link

1Copenut commented Jun 2, 2022

Proposed meter metric

! I moved the progress bar after the text and added a role="meter" to it
! This gives screen readers a data point to read out that correlates with the visual representation

<div
+ aria-labelledby="metric-progressbar-title"
  class="echMetric echMetric--border echMetric--small echMetric--vertical echMetric--progressbar"
  style="background-color: rgb(255, 255, 255);"
+ role="figure"
>
  <!-- Text data -->
  <div class="echMetricText echMetricText--small echMetricText--vertical" style="color: rgb(52, 55, 65);">
    <div>
      <h2
        class="echMetricText__title echMetricText__titleS"
        id="metric-progressbar-title"
      >
        Disk I/O
        <span class="echMetricText__subtitle">Read</span>
      </h2>
    </div>
    <div class="echMetricText--bottom-right">
      <p class="echMetricText__value echMetricText__valueS">
+       <span aria-hidden="true">
+         <span class="echMetricText__unit echMetricText__unit--large">12.57</span>
+         <span class="echMetricText__unit"> Mb/s</span>
+       </span>
+       <span class="sr-only">Twelve point fifty-seven megabits per second</span>
      </p>
    </div>
  </div>

  <!-- Progress bar -->
  <div
    class="echSingleMetricProgress echSingleMetricProgress--vertical echSingleMetricProgress--small"
    style="background: rgb(237, 240, 245);"
  >
    <div
+     aria-label="Input/output capacity in use"
+     aria-valuemin="0"
+     aria-valuemax="100"
+     aria-valuenow="12.57"
      class="echSingleMetricProgressBar echSingleMetricProgressBar--vertical echSingleMetricProgressBar--small"
      style="background: rgb(94, 94, 94); height: 12.57%;"
+     role="meter"
    ></div>
  </div>
</div>

@markov00
Copy link
Member Author

markov00 commented Jun 3, 2022

Hey @1Copenut thanks so much for giving a look at this PR. I was in fact going to ping you on that, to check the accessibility. Please, consider that this is in an alpha stage, so it shouldn't be perfect right away.

I'd like to ask you a few questions:

  1. We unfortunately can't control how the value 323570 is formatted into $ 323.57k. We just apply a conversion to
<h4 class="echMetricText__value echMetricText__valueS"><span class="echMetricText__unit">$ </span>323.57<span class="echMetricText__unit">k</span></h4>

to improve the rendering of such number within the metric. This cause the first issue: we can't translate the value to free text like Three-hundred twenty-three point fifty-seven. Thousand US dollars. We can't even ask the consumer to translate it, because those values are not fixed. So my first question is: what can be done in this context to improve that?
Note that: Kibana is stuck with an old formatting library (called numeraljs) that completely abstract from the most important bits of information to "automatically" translate to free text those values like what is the unit, what is the multiplier, is there prefix, postfix to the number etc.

  1. I'm afraid I can't move the subtitle as being a span of the title due to the grid layout applied, but I can try and see if I can get the same result. Does it works the same if I configure the subtitle within a p tag instead?

  2. I love the role="meter" I didn't know about it. This is easy to implement and have filled automatically. The SVG title and description unfortunately depends on the consumer, so I will expose those props and hope they will fill them with the right text.

@markov00
Copy link
Member Author

markov00 commented Jun 6, 2022

a0df3bc

Hey @1Copenut I think I've addresses most of the issues you have confirmed in a0df3bc
I've updated the storybook example adding the title and description for the sparklines, and it looks fine to me. I'm not super familiar with VoiceOver but it looks like it read out loud correctly the various elements and the eventual titles.
Just wait few minutes to get the branch deployed if you want to test it with the latest changes

@@ -0,0 +1,21 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like that there's very little dependency on selectors, as they are mostly a potential perf optimization, unwarranted for a light duty chart type. As both selectors are lightweigth, I wonder if these can be avoided altogether, in favor of direct access, like with the GL flame

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, but I've found the usual recursive issue with the state update if I remove the getMetricSpecs selector.
For now I think it's fine keeping them like that, but I will definitely look more into that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be resolved. Let's include it in the reconsideration of the internal chart state, events etc. (internal chart protocol)

@markov00
Copy link
Member Author

markov00 commented Jun 8, 2022

few updates:

  • Together with @gvnmagni we decided that we don't want to limit the possibility of having horizontal and vertical progress bar together, and this should be used carefully buy the consumer.
  • having a progress bar together with a trend doesn't force the alignment anymore, because it seems that benefits have the same weight of disadvantages.
  • the API now reflect those changes and is more compact and simplified
  • a single metric story is added to test each all the capabilities (text, trend, bars, colors)
  • the resposiveness is improved. is not perfect (because we still lacking computing the actual number of rows occupied by the title/subtitle) but at least provide a graceful degradation based on the maximum content.

cc @nickofthyme @monfera can you please double check this again?

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look great!

@nickofthyme nickofthyme requested a review from monfera June 8, 2022 15:45
// (undocumented)
chartType: typeof ChartType.Metric;
// (undocumented)
data: (MetricBase | MetricWProgress | MetricWTrend | undefined)[][];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can MetricSpec be generic on the use case? Ternaries have some extra cost compared to generics. Also, why do we need to admit undefined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • MetricSpec with generic: with the current config no. Because you can mix a simple metric with a static background color with one with a trend and with another with a progress bar.

  • why undefined because we allow gaps between metrics in a row/column.

// @alpha (undocumented)
export type MetricSpecProps = ComponentProps<typeof Metric>;

// @public (undocumented)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the docs are for a next PR, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, unfortunately docs are always for the next PR....

Comment on lines +1718 to +1720
trendShape?: MetricTrendShape;
trendA11yTitle?: string;
trendA11yDescription?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the strings non-optional, if necessary, using the empty string if none is supplied?
Also, any reason not to use a default metric trend shape?

Copy link
Member Author

@markov00 markov00 Jun 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can make them optional on the API and required in the code, sure.
I replied too quickly. It is a bit more tricky than that, because this lives within a data array, and the only way to do that is to loop through it before and update the optional params.
From the point of view of the API these are optional, the user doesn't need to fill them out if they don't want to.

Comment on lines +24 to +25
title?: string;
subtitle?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

@@ -0,0 +1,21 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be resolved. Let's include it in the reconsideration of the internal chart state, events etc. (internal chart protocol)

@1Copenut
Copy link

1Copenut commented Jun 9, 2022

Hey @1Copenut I think I've addresses most of the issues you have confirmed in [a0df3bc]

@markov00 I loaded up the latest preview URL and tested your updates with Safari + VoiceOver. All I can say is wow, it's working really well. The figures are announcing themselves with the H2, labels are clear, and moving images to after the text brings the visualization together when it's read back. Much appreciated!

@markov00 markov00 merged commit fc2c955 into elastic:master Jun 10, 2022
@markov00 markov00 deleted the single_metric branch June 10, 2022 07:41
nickofthyme pushed a commit that referenced this pull request Jun 10, 2022
# [46.10.0](v46.9.0...v46.10.0) (2022-06-10)

### Features

* Metric visualization ([#1658](#1658)) ([fc2c955](fc2c955))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:new chart type New chart type related feature request :theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metric + micro combo visualization
8 participants